Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relaxing propTypes for CalendarWeek #1857

Merged
merged 5 commits into from Nov 7, 2019
Merged

Conversation

kevinthepan
Copy link
Contributor

Since Airbnb's own monorepo uses the wrapper PdpCalendarDay around CustomizableCalendarDay, it triggers a TypeError every time the component tree is mounted in any sort of unit test.

Relaxing the propTypes for CalendarWeek to accept any sort of node.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was very intentional; the fix should be made in the internal component. Why does it have to pass a custom kind of CalendarDay in the first place?

@majapw
Copy link
Collaborator

majapw commented Nov 6, 2019

The existing failure is unrelated to this change. @krissalvador27 is looking into it.

@krissalvador27
Copy link
Contributor

Hey @majapw, I have a fix open here #1854 😁

@kevinthepan
Copy link
Contributor Author

@ljharb We have a bunch of business logic surrounding CustomizableCalendarDay (day-specific styling, day contents styling, day dependent phrases, etc) that makes things a lot cleaner and more manageable when encapsulated as part of the day instead of at the date picker level.

@majapw
Copy link
Collaborator

majapw commented Nov 7, 2019

Thanks @krissalvador27!

And yeah, agree with @kevinthepan's statement. I think we are more generally interested in transforming this library into a more flexible solution so as to better accommodate needs both internally and externally.

@majapw majapw merged commit b600767 into react-dates:master Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants